Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (version6@6377b49). Learn more about missing BASE report.

@@             Coverage Diff             @@
##             version6    #4627   +/-   ##
===========================================
  Coverage            ?   73.08%           
===========================================
  Files               ?      479           
  Lines               ?    17389           
  Branches            ?     3431           
===========================================
  Hits                ?    12709           
  Misses              ?     3821           
  Partials            ?      859           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Flash0ver I'm trying to force an analyzer warning for the CA2000 (so that I can build and test a way to suppress this). I've added this to the csproj file for samples/Sentry.Samples.Console.Basic/Sentry.Samples.Console.Basic.csproj:

<WarningsAsErrors>CA2000</WarningsAsErrors>

However I can't seem to get it to work:

The warning does appear for the undisposed use of Foo (which I added to test the warning was enabled)... but we're not seeing it for our newly disposable ISpan classes. StartTransaction returns an ITransactionTracer that descends from ISpan, which implements IDisposable.

Any ideas/thoughts?

Oh ... perhaps I totally misunderstood CA2000 😟

DisposeObjectsBeforeLosingScope.cs

Hmmm ... it seems that the CA2000-DiagnosticAnalyzer does some Control-Flow-Analysis, that either tries to "see through" the implementation and does not report when it can't statically figure it out, or just does not report for some usage patterns.

var instance1 = new MyDisposable(); //CA2000
var instance2 = new MyFactory().CreateInstance(); //CA2000
var instance3 = MyFactory.CreateStatic(); //CA2000
var instance4 = MyFactory.Instance.CreateInstance(); // does not report CA2000

public sealed class MyDisposable : IDisposable
{
    public void Dispose()
    {
    }
}
public interface IMyFactory
{
    IDisposable CreateInstance();
}
public sealed class MyFactory : IMyFactory
{
    public static IMyFactory Instance { get; } = new MyFactory();
    public static IDisposable CreateStatic() => new MyDisposable();
    public IDisposable CreateInstance() => new MyDisposable();
}

Not sure if that analysis behavior is intended or not.
Perhaps the Analyzer cannot "see through" our rather complex control flow from SentrySdk, via the IHub, to some extension-methods.

Now, as a follow-up, I'm wondering:
We do have 2 public types that inherit ISpan, and with that IDisposable:

var transaction = new TransactionTracer(null!, null!); //CA2000
var span = new SpanTracer(null!, transaction, default, default, null!); //CA2000
span.Finish();
transaction.Finish();

Are there valid/intended usage scenarios to instantiate these types directly from user code, which would report CA2000, even when properly Finished?
Or ... perhaps we do not need a DiagnosticSuppressor for CA2000, since it's not reported for our implementation ... which may change in the future 🤔

Are there valid/intended usage scenarios to instantiate these types directly from user code, which would report CA2000,

I can't think of any scenario in which people would want to construct those manually... so I suspect not.

Or ... perhaps we do not need a DiagnosticSuppressor for CA2000, since it's not reported for our implementation ...

That's possibly the case yes... and without being able to reproduce the warning that it's supposed to suppress, it's relatively difficult to test. I think this PR might be ready for review then...